Skip to content

fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94)#113

Open
beardthelion wants to merge 11 commits into
mainfrom
fix/gate-repo-read-surfaces
Open

fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94)#113
beardthelion wants to merge 11 commits into
mainfrom
fix/gate-repo-read-surfaces

Conversation

@beardthelion

@beardthelion beardthelion commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Closes #94, and gates the three sibling read surfaces that share its pattern.

Problem

Four GET handlers on the node sat on the optional_signature read group with no ownership or visibility check, so an unauthenticated caller who knew a repo's owner/name could read data that should be private:

Fix

Gating model follows data sensitivity, not a blanket rule:

  • list_webhooks is owner-restricted but layered read-visibility-then-owner: headerless -> 401, non-reader of a private repo -> 404 (existence hidden, uniform with the siblings), non-owner of a public repo -> 403. Webhook callback URLs are owner-secret config.
  • list_replicas, list_protected_branches, list_repo_events gate on read visibility (authorize_repo_read at /, the INV-2 root-listing exception). Public repos stay anonymously readable; private repos return an opaque 404. replicas is documented as a public mirror-discovery surface, so read-visibility (not owner-only) preserves that for public repos.
  • list_repo_events only gates the locally-hosted branch: when the node holds the repo it runs visibility_check on the loaded record before serving either the cert or gossip events; a repo known only via gossip (no local row) keeps its existing public federation-feed behavior.

Each new gate is pinned into the existing authz_guard drift table so it cannot silently regress.

CLI/MCP

Tightening the node broke gl/MCP read commands that sent unsigned requests. Fixed the callers of the newly gated endpoints: gl webhook list and the MCP webhook_list tool now sign (get_signed); gl protect list, gl repo replicas, and the replica/branch sub-fetches in gl repo info/gl repo owner sign when an identity is present via a new NodeClient::get_maybe_signed (anonymous otherwise, so public repos still work without auth). Added --dir to gl repo replicas.

Tests

Test-first throughout. Coverage includes the full per-surface matrix (owner / non-owner / reader / stranger / headerless / anonymous / absent), the no-existence-oracle property (headerless on an existing-private vs an absent repo both 401), the gossip-only vs local-private split for events, both branches of get_maybe_signed, an end-to-end test that runs a real sign_request output through the node's optional_signature middleware, and signature-header assertions on the CLI/MCP signing paths.

Deferred (separate issues)

  • The global GET /api/v1/events/ref-updates feed is ungated for private-repo ref metadata (the REST analog of GraphQL refUpdates serves private-repo ref metadata to anonymous callers (visibility bypass) #112). Out of scope here; kept to the per-repo surfaces.
  • Several other gl/MCP read commands (gl repo commits, gl pr *, MCP repo_get/pr_*) hit endpoints that were already visibility-gated before this change and send unsigned requests, so they are broken for private repos independently of this PR. Worth a follow-up to sign all gl/MCP read commands.

Summary by CodeRabbit

  • New Features
    • Added identity-aware (optional) request signing for CLI reads, so requests are signed when credentials are available.
    • Added --dir support to gl repo replicas and gl webhook list to enable signed, owner-gated access.
  • Bug Fixes
    • Enforced read-visibility gating for protected branches, replicas, and webhooks on private repositories, returning 401/403/404 appropriately and redacting secret fields.
    • Improved list_repo_events privacy by fail-closed handling for local repo lookup errors to avoid unintended metadata leakage.
  • Tests
    • Expanded integration/MCP coverage for signed webhook listing and visibility-gated endpoints.

list_webhooks sat on the optional_signature read group with no auth or
ownership check, so any unauthenticated caller who knew a repo's owner/name
could enumerate its webhooks (target URL, created_by_did, events; only the
secret was redacted).

Gate it read-visibility-then-owner, mirroring create_webhook/delete_webhook:
headerless -> 401, non-reader of a private repo -> 404 (existence hidden,
uniform with the sibling read surfaces), non-owner of a public repo -> 403.
Add a list_webhooks row to the authz_guard drift table so the gate can't
silently regress.
list_replicas sat on optional_signature with no visibility check, so a
private repo's replica URLs were enumerable by anyone who knew owner/name.
Replica lists are a documented public mirror-discovery surface, so gate on
read visibility (authorize_repo_read) rather than owner-only: a public repo
stays anonymously listable, a private repo returns 404 to anyone who cannot
read it.

register_replica registers non-owner DIDs, and a replica operator is not a
visibility reader, so a non-owner replica operator of a private repo gets
404 — the intended contract, pinned by the new test. Add the authz_guard
row.
list_protected_branches sat on optional_signature with no visibility check,
leaking a private repo's branch names to any unauthenticated caller. Gate on
read visibility (authorize_repo_read at the root, INV-2 root-listing
exception): public repos stay anonymously listable, private repos 404 to a
non-reader. Add the authz_guard row.
…-only (#94)

list_repo_events served a private repo's ref certificates and gossip ref
metadata (ref names, SHAs) to any unauthenticated caller. The handler also
tolerates a repo it knows only via gossip (no local row) and legitimately
serves its events, so a blanket get_repo->authorize_repo_read swap would have
404'd that federation path.

Gate only the locally-hosted branch: when get_repo returns a record, run
visibility_check on it at the root and 404 on deny before fetching either the
cert or gossip events. A gossip-only repo (no local row) keeps its existing
public federation-feed behavior; the global /api/v1/events/ref-updates feed
remains a separately tracked deferral. Add the authz_guard row (visibility_check
marker, since the gate is direct to avoid a second get_repo).
…ated (#94)

`gl webhook list` built its NodeClient with no keypair, so the GET went out
anonymously. With the node now owner-gating GET /hooks, that request returns
401. Thread a --dir through the List subcommand and load the keypair, matching
`gl webhook create`/`delete`, so the listing is signed as the owner.

(Replica and protected-branch listings stay anonymous for public repos under
read-visibility gating, so their gl clients need no change.)
Code review caught that NodeClient::get() never attaches the RFC 9421
signature even when a keypair is loaded — only get_signed/post/put/delete
sign. The earlier gl webhook-list fix loaded the keypair but still called
get(), so `gl webhook list` (and the MCP webhook_list tool) 401'd against
the now-owner-gated endpoint. Switch both to get_signed.

Harden the two cmd_list tests to require a Signature/Signature-Input header on
the mock (verified: they fail against plain get(), pass with get_signed) — the
missing assertion that let the bug through. Add the untested owner-of-private-
repo webhooks path (200 + redaction). Fix the stale module doc and the
authz_guard bucket comment for list_webhooks.
…eir own repos (#94)

The #94 server change read-visibility-gates GET /replicas and
/branches/protected: public repos stay anonymous, private repos require the
owner/reader to authenticate. Four gl read commands sent unsigned GETs and so
broke (404) or silently degraded (empty output) for a private repo's owner:
gl protect list, gl repo owner (silent empty protected list), gl repo info
(silent missing replica count), gl repo replicas (and it had no --dir flag).

Add NodeClient::get_maybe_signed — signs when an identity is present, stays
anonymous otherwise (mirrors the conditional signing of post/put/delete) —
and route the four call sites through it. Add --dir to `gl repo replicas`.
Harden the protect-list tests to assert the request is signed (verified: they
fail on plain get()). Public-repo anonymous reads are unchanged.
Round-2 review caught that cmd_info and cmd_owner loaded a keypair and signed
their replica/protected-branch sub-fetch but left the PRIMARY GET
/api/v1/repos/{owner}/{name} on plain get(). get_repo is read-visibility-gated,
so for a private repo that primary fetch 404'd first and the command bailed
before reaching the signed sub-fetch — the half-fix never actually let an owner
inspect their own private repo. Route both primary fetches through
get_maybe_signed.

Add the missing signing guards: harden test_cmd_owner_is_owner to require a
signature on both mocks, and add tests for cmd_replicas and cmd_info (both were
untested) asserting the request is signed when an identity is present.

Out of scope (pre-existing, gated before #94, untouched here): gl repo commits,
gl pr list/view/diff, and the MCP repo_get/repo_commits/pr_* tools call other
already-gated read endpoints unsigned and are broken for private repos
independently of this change.
Close the vetting gaps from the review:
- node: a listed reader (non-owner) passes the read gate but is still refused
  the webhook list (403), and headerless on an existing-private vs an absent
  repo both 401 — proving the 401 fires before any lookup, so it is not an
  existence oracle.
- node: the read-visibility surfaces (replicas, protected branches) admit a
  listed reader who is not the owner (the allow-list branch of visibility_check),
  while a non-reader stranger still 404s.
- gl: get_maybe_signed's anonymous fallback (no identity -> unsigned GET) is now
  exercised, RED-proven by flipping it to get_signed (which signs and fails the
  no-signature-header assertion).
)

Close the last two reasoned-only paths by execution:

- MCP webhook_list: a call_tool("webhook_list", ...) dispatch test against a
  mock node asserts the /hooks request carries a signature (get_signed).
  RED-proven: flipping the arm back to get() fails the header assertion.

- gl->node end-to-end: a real RFC-9421 signature produced exactly as the gl
  client's get_signed does (gitlawb_core::http_sig::sign_request over GET +
  empty body) is run through the node's actual optional_signature middleware,
  which verifies it and authorizes the owner (200 + webhook body). RED-proven:
  signing over POST while sending GET makes the node reject it. This stitches
  the gl signing side and the node verifying side in one test rather than
  mocking one end.
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Node read endpoints now enforce visibility or ownership checks for webhooks, replicas, protected branches, and repo events. CLI read paths for webhooks, repo info, replicas, protected branches, and MCP webhook listing now conditionally sign GET requests when an identity keypair is available.

Changes

Node-side endpoint authorization

Layer / File(s) Summary
Endpoint authorization changes
crates/gitlawb-node/src/api/webhooks.rs, crates/gitlawb-node/src/api/replicas.rs, crates/gitlawb-node/src/api/protect.rs, crates/gitlawb-node/src/api/events.rs
list_webhooks, list_replicas, list_protected_branches, and list_repo_events now accept authenticated caller context and apply visibility or ownership checks before returning data.
Gate markers and integration tests
crates/gitlawb-node/src/api/mod.rs, crates/gitlawb-node/src/test_support.rs
The authz guard fixture list includes events.rs, marker expectations cover the updated handlers, and integration tests validate webhook, replica, protected-branch, and event visibility behavior.

CLI signed request support

Layer / File(s) Summary
Conditional signing helper
crates/gl/src/http.rs
NodeClient gains get_maybe_signed, which signs GET requests with RFC 9421 headers when a keypair exists and otherwise sends an anonymous GET.
Webhook list signing
crates/gl/src/webhook.rs
gl webhook list accepts --dir, forwards it through dispatch, and uses a signed request for the hooks endpoint, with tests checking signature headers.
Repo read signing
crates/gl/src/repo.rs
gl repo adds --dir for replicas, loads identities for repo info, replicas, and owner reads, and updates tests to cover signed and unsigned request paths.
Protected branches and MCP signing
crates/gl/src/protect.rs, crates/gl/src/mcp.rs
gl protect list switches to conditional signing, and the MCP webhook_list tool uses a signed request with a matching test.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • Gitlawb/node#25: Introduced visibility_check and the visibility primitives used by the new repo-event gating.
  • Gitlawb/node#52: Added authorize_repo_read, which this PR now applies to multiple read endpoints.
  • Gitlawb/node#87: Covers the per-route authorization enforcement pattern extended here.

Suggested labels

subsystem:identity

Suggested reviewers

  • kevincodex1
  • jatmn

Poem

🐇 I hop through gates with a signed little grin,
Webhooks, replicas, and branches tucked in.
Events stay hidden when visibility says no,
But trusted callers still get the show.
A carrot for auth, and a signature cheer—
The rabbit approves: safe reads are here!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific, concise, and matches the main change: gating the /hooks endpoint and related private-repo read surfaces.
Description check ✅ Passed The description covers the problem, fix, tests, and deferred follow-ups, even though it doesn't mirror the template exactly.
Linked Issues check ✅ Passed The webhook list endpoint now requires auth, read visibility, and owner access while preserving secret redaction, matching #94.
Out of Scope Changes check ✅ Passed The client signing and sibling endpoint updates are part of the stated fix and not unrelated churn.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/gate-repo-read-surfaces

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/gl/src/webhook.rs (1)

157-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Check the webhook-list HTTP status before reading webhooks.

A 401/403/404 JSON error body has no webhooks field, so this currently prints “No webhooks” and exits successfully instead of surfacing the authorization failure.

Proposed fix
-    let resp: Value = client
+    let resp = client
         .get_signed(&format!("/api/v1/repos/{owner}/{repo}/hooks"))
-        .await?
-        .json()
-        .await
-        .context("invalid JSON")?;
+        .await?;
+    let status = resp.status();
+    let resp: Value = resp.json().await.unwrap_or_default();
+    if !status.is_success() {
+        let msg = resp["message"].as_str().unwrap_or("unknown error");
+        anyhow::bail!("webhook list failed ({status}): {msg}");
+    }
 
     let hooks = resp["webhooks"].as_array().cloned().unwrap_or_default();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gl/src/webhook.rs` around lines 157 - 164, The webhook listing flow in
`list_webhooks` is treating error responses as empty results because it parses
JSON and reads `resp["webhooks"]` without checking the HTTP status first. Update
the `client.get_signed(...).await?` handling to inspect the response status
before deserializing or accessing `webhooks`, and surface 401/403/404 as errors
instead of falling back to `No webhooks`. Keep the fix localized around the
`list_webhooks` response handling and the `webhooks` extraction logic.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/mod.rs (1)

177-181: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Guard both halves of list_webhooks authorization.

This row only enforces the owner gate. Add a second marker for authorize_repo_read( so the drift test also catches regressions that reintroduce private-repo existence leaks before require_repo_owner.

🧪 Proposed guard hardening
             // Bucket A/B hybrid — list_webhooks is read-visibility THEN owner:
             // authorize_repo_read 404s a non-reader of a private repo, then
             // require_repo_owner 403s a non-owner of a public one. The
             // require_repo_owner marker guards the owner half.
+            (webhooks, "list_webhooks", "authorize_repo_read("),
             (webhooks, "list_webhooks", "require_repo_owner("),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/api/mod.rs` around lines 177 - 181, The
`list_webhooks` drift-test row in `api::mod` only checks the owner gate, so it
can miss regressions in the read-visibility half. Update the marker set for
`list_webhooks` to include both `authorize_repo_read(` and `require_repo_owner(`
so the test covers the private-repo existence check as well as the owner check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/api/events.rs`:
- Around line 66-80: The repo visibility gate in events handling is failing open
because `repo_record` is built with `.ok().flatten()`, so a database lookup
error is indistinguishable from a missing local repo and skips the
`visibility_check` path. Update the `events` flow to preserve and propagate the
lookup error from the local repo fetch, and only allow the ungated gossip-only
behavior when the lookup successfully returns `Ok(None)`. Keep the existing
`visibility_check`/`RepoNotFound` logic in the `repo_record`-based branch so
`api::events` fails closed for local private repos.

---

Outside diff comments:
In `@crates/gl/src/webhook.rs`:
- Around line 157-164: The webhook listing flow in `list_webhooks` is treating
error responses as empty results because it parses JSON and reads
`resp["webhooks"]` without checking the HTTP status first. Update the
`client.get_signed(...).await?` handling to inspect the response status before
deserializing or accessing `webhooks`, and surface 401/403/404 as errors instead
of falling back to `No webhooks`. Keep the fix localized around the
`list_webhooks` response handling and the `webhooks` extraction logic.

---

Nitpick comments:
In `@crates/gitlawb-node/src/api/mod.rs`:
- Around line 177-181: The `list_webhooks` drift-test row in `api::mod` only
checks the owner gate, so it can miss regressions in the read-visibility half.
Update the marker set for `list_webhooks` to include both `authorize_repo_read(`
and `require_repo_owner(` so the test covers the private-repo existence check as
well as the owner check.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ee75b2eb-a906-4a01-b6ce-0fd597b7eec6

📥 Commits

Reviewing files that changed from the base of the PR and between 6ae316c and e2ba8a8.

📒 Files selected for processing (11)
  • crates/gitlawb-node/src/api/events.rs
  • crates/gitlawb-node/src/api/mod.rs
  • crates/gitlawb-node/src/api/protect.rs
  • crates/gitlawb-node/src/api/replicas.rs
  • crates/gitlawb-node/src/api/webhooks.rs
  • crates/gitlawb-node/src/test_support.rs
  • crates/gl/src/http.rs
  • crates/gl/src/mcp.rs
  • crates/gl/src/protect.rs
  • crates/gl/src/repo.rs
  • crates/gl/src/webhook.rs

Comment thread crates/gitlawb-node/src/api/events.rs

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Fail closed when the local repo lookup errors
crates/gitlawb-node/src/api/events.rs:64
CodeRabbit's inline review item is still valid: get_repo errors are currently collapsed with .ok().flatten(), so a database error is indistinguishable from Ok(None) and the handler takes the ungated gossip-only path. That means the new private-repo visibility gate only runs when the lookup succeeds; on a lookup error, the handler can continue into the public event feed path instead of failing closed. Please complete that review request by propagating the lookup error and only using the gossip-only behavior for an actual Ok(None).

[P2] Surface webhook-list authorization failures instead of printing an empty list
crates/gl/src/webhook.rs:157
CodeRabbit's outside-diff review item is still valid: gl webhook list signs the request now, but it immediately deserializes the response and reads resp["webhooks"] without checking the HTTP status. A 401/403/404 JSON error body has no webhooks field, so the command prints No webhooks for and exits successfully instead of telling the user that the node rejected the request. Please complete that review request by checking resp.status() before extracting webhooks, matching the status handling already used by the neighboring list commands.

[P3] Pin both halves of the list_webhooks authorization gate
crates/gitlawb-node/src/api/mod.rs:181
CodeRabbit's drift-guard comment is still valid: the authz_guard row for list_webhooks only checks for require_repo_owner(, even though this route depends on authorize_repo_read( first to hide private-repo existence before the owner-only check runs. The behavior tests cover the current implementation, but the static guard can still pass if the read-visibility half is later removed. Please add the authorize_repo_read( marker for list_webhooks too, so the regression guard matches the layered security boundary this PR is adding.

@beardthelion beardthelion added kind:security Vulnerability fix or hardening crate:node gitlawb-node — the serving node and REST API subsystem:api Node REST API request/response surface subsystem:visibility Path-scoped visibility and content withholding sev:high Major break or real security/trust risk, no easy workaround labels Jun 29, 2026
…nts (#94)

`get_repo(...).await.ok().flatten()` collapsed a lookup Err into None, so a
transient DB failure skipped the read-visibility gate and the handler served a
private repo's gossip ref-update events. Propagate with `?` instead: an error
now maps to AppError::Internal (500) and fails closed, while a genuine Ok(None)
(repo not hosted locally) stays the intentional ungated pass-through.

Add a regression test that forces a get_repo error (drop the column its SELECT
reads) and asserts 500 with no secret in the body. The injection is
self-verifying: it asserts the lookup succeeds pre-drop and errors post-drop, so
the test can't pass vacuously if get_repo's projection changes later.
@beardthelion beardthelion requested a review from jatmn June 29, 2026 01:36

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/api/events.rs (1)

55-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use did_matches(...) for the owner fast-path behind this new gate.

This handler now relies on visibility_check to admit the repo owner, but that helper still treats ownership as caller == owner_did || caller == owner_short. If a local repo row is stored in bare form, a signed did:key:... owner is denied here and gets the opaque 404 reserved for non-readers.

Based on learnings, owner DID comparisons must normalize bare and full forms with did_matches(...).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/api/events.rs` around lines 55 - 87, The new
visibility gate in the events handler is still relying on raw owner DID
equality, which can wrongly deny the repo owner when the stored owner is in bare
form and the caller is a full DID. Update the owner fast-path in the events flow
by using did_matches(...) for the ownership check, and apply it consistently in
the visibility_check path around the repo_record handling so bare and full DID
forms both match correctly.

Source: Learnings

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/gitlawb-node/src/api/events.rs`:
- Around line 55-87: The new visibility gate in the events handler is still
relying on raw owner DID equality, which can wrongly deny the repo owner when
the stored owner is in bare form and the caller is a full DID. Update the owner
fast-path in the events flow by using did_matches(...) for the ownership check,
and apply it consistently in the visibility_check path around the repo_record
handling so bare and full DID forms both match correctly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dc377dcf-7169-4355-96cd-8ce371715b57

📥 Commits

Reviewing files that changed from the base of the PR and between e2ba8a8 and 8453814.

📒 Files selected for processing (2)
  • crates/gitlawb-node/src/api/events.rs
  • crates/gitlawb-node/src/test_support.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crate:node gitlawb-node — the serving node and REST API kind:security Vulnerability fix or hardening sev:high Major break or real security/trust risk, no easy workaround subsystem:api Node REST API request/response surface subsystem:visibility Path-scoped visibility and content withholding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unauthenticated GET /repos/:owner/:repo/hooks leaks webhook target URLs for any repo

2 participants